Tagging subscribers to this area: @JulieLeeMSFT, @dotnet/jit-contrib
See info in area-owners.md if you want to be subscribed.

There seems to be false-positives:

https://helix.dot.net/api/jobs/beac8f40-f00e-422f-b8e1-7f4132c2d21c/workitems/JIT.CodeGenBringUpTests_do?api-version=2019-06-17

Error: Input file not found: -DET
.NET IL Assembler

Error: Input file not found: -DET
.NET IL Assembler

Error: Input file not found: -DET
.NET IL Assembler

Error: Input file not found: -DET
.NET IL Assembler

Error: Input file not found: -DET
.NET IL Assembler

perhaps it is not failing with non-zero exit code, or perhaps test infra is not listening to the error code?

This should fix the issue by adding support for modern/command-line-api style switches: CLRTest.Jit.targets.txt

The exit code is 1, but test/helix infra doesn't consider it as failure. It's a separate issue, but worth noticing.

@am11 I have another PR I'm going to put out with a bunch of fixes that for some reason this didn't catch (with regression tests). I'll switch this into draft and we can come back to it later.

@copilot please ensure that the generated round-trip script exits with a failure code (and fails the test) if the ilasm execution fails with an error.

Also, please address the review feedback.

@copilot please ensure that the generated round-trip script exits with a failure code (and fails the test) if the ilasm execution fails with an error.

Also, please address the review feedback.

Done in commit 488df8b. Here's what was addressed:

Round-trip script exits on failure (your request): All ildasm and ilasm subprocess calls now check proc.returncode != 0 and call sys.exit(1) with a descriptive error message. Previously, ilasm failures were silently swallowed (the test_det flag just skipped determinism testing rather than failing the test).

Review feedback:

• run.cmd: Added set __UseManagedIlasm= initialization at the top alongside other flags. usemanagedilasm now also sets __IlasmRoundTrip=1 so it can never be a no-op.
• run.py: When --use_managed_ilasm is specified without --ilasmroundtrip, it now auto-enables round-trip by setting RunningIlasmRoundTrip=1 directly (with an explanatory message).
• testenvironment.proj: Updated the header comment to call out both RunningIlasmRoundTrip and IlasmRoundTripUseManagedIlasm as the non-DOTNET_ variable exceptions.

@copilot please mark the test mentioned in #127576 as il round-trip incompatible with a reference to the issue.

Also, please update src\tests\Interop\IJW\Directory.Build.props to mark all tests in that subtree as il round-trip incompatible

@copilot please mark the test mentioned in #127576 as il round-trip incompatible with a reference to the issue.

Also, please update src\tests\Interop\IJW\Dir...

Done in commit 1a576ca:

• Loader/classloader/regressions/dev10_403582/dev10_403582.ilproj: added <IlasmRoundTripIncompatible>true</IlasmRoundTripIncompatible> with a comment referencing ILASM round trip failing for Loader/classloader/regressions/dev10_403582/dev10_403582/dev10_403582.cmd test #127576 (ildasm crashes on genmeth.dll).
• src/tests/Interop/IJW/Directory.Build.props: added <IlasmRoundTripIncompatible>true</IlasmRoundTripIncompatible> to mark all IJW tests in that subtree as incompatible, with a comment explaining that IJW assemblies contain native code that cannot be round-tripped through ilasm.

🤖 Copilot Code Review — PR #127096

Holistic Assessment

Motivation: The PR adds infrastructure to run ilasm round-trip tests with the new managed ilasm. This is justified scaffolding to validate the managed ilasm rewrite, with the CI scenario correctly disabled until dependencies are ready (#127297).

Approach: Clean and minimal — an environment variable switches the ilasm path, a new test scenario wires it up via testenvironment.proj, and the existing round-trip Python script is extended. The secondary improvement of failing fast on ILDASM/ILASM errors (instead of silently skipping) is a welcome bonus.

Summary: ⚠️ Needs Human Review. The code is correct for the scenario-based CI path (the primary use case), but run.sh is missing the --use_managed_ilasm flag that both run.cmd and run.py have, creating a platform parity gap for local testing on Linux/macOS. A human reviewer should decide whether this gap is acceptable for initial merge or should be addressed in this PR.

Detailed Findings

⚠️ Missing --use_managed_ilasm in run.sh — Platform parity gap (flagged by all 3 models)

run.cmd (line 77) supports usemanagedilasm and run.py (line 104) supports --use_managed_ilasm, but run.sh (the Linux/macOS wrapper) has no corresponding flag. Only --ilasmroundtrip is handled (lines 146-148).

Evidence: Searched run.sh for managed_ilasm, managedilasm, ManagedIlasm — zero results. Compared with run.cmd line 77 (set __IlasmRoundTrip=1&set __UseManagedIlasm=1) and run.py line 104 (parser.add_argument("--use_managed_ilasm", ...)).

Impact: Linux/macOS developers cannot invoke managed ilasm round-trip via the shell wrapper. Since CI uses scenarios from testenvironment.proj (which correctly defines managedilasmroundtrip), this doesn't block CI. However, it breaks the PR's stated goal of "CLI support for local testing."

Classification: Advisory — not merge-blocking since CI works through scenarios, but inconsistent with the PR's own description. A follow-up is acceptable if the author prefers.

Suggested fix (around run.sh line 148):

        --use_managed_ilasm|--usemanagedilasm)
            ((ilasmroundtrip = 1))
            runtestPyArguments+=("--use_managed_ilasm")
            ;;

⚠️ Missing usemanagedilasm in run.cmd help text (flagged by GPT-5.3-codex)

run.cmd implements the usemanagedilasm option (line 77) but does not document it in the :Usage section (lines 268-279). This makes the switch undiscoverable for users running run.cmd /?.

Classification: Advisory — not merge-blocking but should be added for completeness.

✅ Error handling improvement — Fail-fast on ILDASM/ILASM errors

The change from silently continuing on failure (test_det = proc.returncode == 0 gating determinism tests) to exiting immediately with a clear error message is a solid improvement. Previously, a failing ilasm would silently skip determinism testing, potentially hiding real issues.

✅ Test exclusions are well-justified

• IJW tests (Interop/IJW/Directory.Build.props): Correctly marked IlasmRoundTripIncompatible — IJW assemblies contain native code that can't round-trip through ilasm.
• dev10_403582 (dev10_403582.ilproj): Marked with a tracking issue (#127576) for the ildasm crash on genmeth.dll.

✅ run.cmd and run.py properly address earlier review feedback

The earlier Copilot review (from commit f0baccf61) flagged that usemanagedilasm didn't imply ilasmroundtrip and __UseManagedIlasm wasn't initialized. Commit 488df8b7f addressed both:

• run.cmd line 77: usemanagedilasm now sets both __IlasmRoundTrip=1 and __UseManagedIlasm=1
• run.cmd line 31: __UseManagedIlasm= is now initialized
• run.py lines 833-840: --use_managed_ilasm auto-enables ilasmroundtrip with a helpful message
• testenvironment.proj comment updated to document both non-DOTNET variables

✅ CI scenario correctly disabled in final commit

The last commit (e22bde4b7) removes managedilasmroundtrip from the ilasm test group in run-test-job.yml, keeping the infrastructure in place while avoiding CI failures until #127297 is merged.

✅ Cross-platform path handling is correct

os.path.join(CORE_ROOT, "managed-ilasm", "ilasm") in CLRTest.Jit.targets lines 340-343 matches the artifact layout in CoreRootArtifacts.targets lines 109-112. The shell=True subprocess invocation handles platform-appropriate executable resolution (.exe on Windows).

💡 Minor: Python indentation inconsistency in embedded script

The ilasm_path selection (lines 340-343) uses 2-space indent while error handling inside the loop uses 4-space. This matches the pre-existing mixed style in the file, so it's not a regression.

Generated by Code Review for issue #127096 · ◷

@copilot please disable the failing hugeexpr test for ilasm round trip testing (and mark as RequiresProcessIsolation if not already)

@copilot please disable the failing hugeexpr test for ilasm round trip testing (and mark as RequiresProcessIsolation if not already)

Done in commit 112c3d6. Added <IlasmRoundTripIncompatible>true</IlasmRoundTripIncompatible> to hugeexpr1.csproj. RequiresProcessIsolation was already set on this test.